Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added command option to disable type-casting #268

Open
wants to merge 48 commits into
base: master
Choose a base branch
from

Conversation

laurent22
Copy link
Contributor

By default, the underlying command parser package (minimist), converts strings that look like numbers to actual numbers. This causes all kind of problems since it's also going to randomly convert things like hashes to numbers.

For example, a hash like "a1ef" would result in "a1ef", but another hash like "93e8" would result in "9300000000". There are quite a few pull requests about this in the minimist package GitHub page, but unfortunately it's no longer maintained so it looks like it won't be fixed.

This pull request attempts to fix this in a way that preserves backward compatibility. It adds a command option disableTypeCasting() which, when used, disables type casting for all arguments and all flags for that command. If that option is not set, the code path is exactly the same so the applications that don't explicitly opt-in won't be affected.

jbrumwell and others added 30 commits September 23, 2016 17:17
Conflicts:
	dist/command.js
	dist/option.js
	lib/command.js
Updated dev deps and switched to Yarn
Updated Inquirer to v3 and other prod deps
…values

Support default values for options
Update babel/eslint build config
Under some rare conditions, Vorpal can produce an invalid persisted history which, when loaded, would result in an uncaught exception that crashes the caller. To prevent this, the history is reset when invalid JSON is loaded.
Fixed crash when persisted history cannot be parsed
@@ -56,7 +56,7 @@ export default function buildCommandArgs(
});

// Use minimist to parse the args, and then build varidiac args and options.
const parsedArgs = parseArgs(passedArgs, types);
const parsedArgs = parseArgs(passedArgs, types, command._disableTypeCasting === true);
Copy link
Contributor

@milesj milesj Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense to remove the type casting option from this types object? (types are options passed to minimist: https://github.com/substack/minimist#var-argv--parseargsargs-opts).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could indeed provide access to the types array, but I think that's too low level, and it requires users to manually add each flag to this list (and to do it again whenever a flag change). This disableTypeCasting() method is higher level and easier to use. It's even possible to disable type casting and then to manually do type casting in the action method.

Also for users who don't want type casting at all anywhere, it makes it easier to disable it by looping on the commands and calling this method.

This is a bit similar to what Vorpal is already doing for flags without arguments, where it adds them automatically to types.booleans for convenience.

@laurent22
Copy link
Contributor Author

Is there any chance this pull request could be merged in one form or another?

I keep running into issues due to Minimist automatic casting. For example, most of the time the arguments are going to be strings so it's fine to use functions like "indexOf" on them, but in rare cases the argument is going to be converted to a number, which makes string functions suddenly fail.

So I need to cast to strings every time when getting an argument from Vorpal. You basically can't expect any consistency and It's quite a annoying bug that can remain undetected for a long time in a program.

Is there maybe some changes I could make to the pull request to get it accepted?

@milesj
Copy link
Contributor

milesj commented Jul 23, 2017

@laurent22 The master branch is currently in development for v2, so merging this in and tagging a new release isn't possible at this time. We're also planning to remove minimist all together.

@milesj
Copy link
Contributor

milesj commented Jul 25, 2017

I moved 2.0 code to a branch and reset master. Sorry, but you'll need to rebase or start this PR again.

@laurent22
Copy link
Contributor Author

Ok that's good news actually if you are going to remove minimist. Is there any roadmap for v2.0? (just to get some idea of what's coming and maybe contribute)

@milesj
Copy link
Contributor

milesj commented Jul 27, 2017

There's a very high level overview here: #234

And the actual refactor PR here: #272

@troggy
Copy link

troggy commented Mar 14, 2018

any workaround? I would like to preserve my input as string, not number

@troggy
Copy link

troggy commented Mar 14, 2018

For those looking for solution. This will make all arguments of the command to be parsed as strings:

.types({
  string: ['_']
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants